errors: move non-frame error types to scylla crate and adjust their visibility #1074

muzarski · 2024-09-18T03:35:50Z

Ref: #519
Depends on #1067

Motivation

Most of the error types defined in scylla-cql::errors module should not belong there. Only 3 types are directly used in scylla-cql crate (DbError, OperationType and WriteType). The rest can be safely moved to scylla crate. It would allow us to modify the visibility of error types much easier. In scylla-cql crate, all error types need to be public - we would need to adjust their pub-visibility via re-exporting in scylla crate which is a tedious work. In addition, it does not make much sense for e.g. QueryError to reside in scylla-cql.

Changes

moved most of the error types from scylla_cql::errors to scylla::errors module
adjusted pub-visibility of some error types. Especially, made transition error types, such as UserRequestError, pub(crate)
moved DbError, OperationType and WriteType types to frame::response::error module. There is no need for additional scylla_cql::errors module just for them
removed scylla_cql::errors module altogether
bonus: adjusted/added some docstrings for public error types
bonus: added more context to TranslationError

Questions

Q: In result, the users can now find non-frame error types under scylla::errors instead of scylla::transport::errors. I'm not sure whether it's a good change or not. Should I move the errors module to scylla::transport module, so we don't introduce too many API breaking changes for the users?

A: We decided to retain the original path. I'm moving the errors module to scylla::transport in last commit.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
~~[ ] I added relevant tests for new features and bug fixes.~~
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
~~[ ] I have adjusted the documentation in ./docs/source/.~~
I added appropriate Fixes: annotations to PR description.

github-actions · 2024-09-18T03:41:00Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 601417c

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev d08018b034b47bb94ff907ee711e35b2060e6f1f
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev d08018b034b47bb94ff907ee711e35b2060e6f1f
     Cloning d08018b034b47bb94ff907ee711e35b2060e6f1f
     Parsing scylla v0.14.0 (current)
      Parsed [  22.326s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  20.335s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.097s] 89 checks: 89 pass, 0 skip
     Summary no semver update required
    Finished [  42.810s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  10.157s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.124s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.099s] 89 checks: 86 pass, 3 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/enum_missing.ron

Failed in:
  enum scylla_cql::errors::BadKeyspaceName, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:538
  enum scylla_cql::errors::BadQuery, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:417
  enum scylla_cql::errors::TranslationError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:344
  enum scylla_cql::errors::BrokenConnectionErrorKind, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:683
  enum scylla_cql::errors::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:22
  enum scylla_cql::errors::CqlResponseKind, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:445
  enum scylla_cql::errors::ConnectionSetupRequestErrorKind, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:609
  enum scylla_cql::errors::CqlRequestKind, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:386
  enum scylla_cql::errors::UserRequestError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:81
  enum scylla_cql::errors::OperationType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:353
  enum scylla_cql::errors::ConnectionError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:558
  enum scylla_cql::errors::WriteType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:361
  enum scylla_cql::errors::NewSessionError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:475
  enum scylla_cql::errors::ResponseParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:728
  enum scylla_cql::errors::DbError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:105
  enum scylla_cql::errors::CqlEventHandlingError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:714
  enum scylla_cql::errors::ConnectionPoolError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:660
  enum scylla_cql::errors::RequestError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:746

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/module_missing.ron

Failed in:
  mod scylla_cql::errors, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:1

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/struct_missing.ron

Failed in:
  struct scylla_cql::errors::ConnectionSetupRequestError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:600
  struct scylla_cql::errors::BrokenConnectionError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-d08018b034b47bb94ff907ee711e35b2060e6f1f/871bee43fb37e82d8690dd6409bb7fa85a905c5d/scylla-cql/src/errors.rs:673

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  20.436s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

wprzytula · 2024-09-18T05:48:01Z

In result, the users can now find non-frame error types under scylla::errors instead of scylla::transport::errors. I'm not sure whether it's a good change or not. Should I move the errors module to scylla::transport module, so we don't introduce too many API breaking changes for the users?

Let's retain module path compatibility for now (i.e., please stick to crate::transport::errors). Once I revive the modules refactor, we'll clean up all module paths.

muzarski · 2024-09-18T06:10:43Z

In result, the users can now find non-frame error types under scylla::errors instead of scylla::transport::errors. I'm not sure whether it's a good change or not. Should I move the errors module to scylla::transport module, so we don't introduce too many API breaking changes for the users?

Let's retain module path compatibility for now (i.e., please stick to crate::transport::errors). Once I revive the modules refactor, we'll clean up all module paths.

Done. I did it in a separate commit at the end. I hope it's acceptable, since rebasing and moving contents of each commit to other file would be a tedious work...

scylla/src/errors.rs

scylla-cql/src/errors.rs

scylla-cql/src/frame/response/mod.rs

scylla-cql/src/frame/response/error.rs

scylla/src/errors.rs

muzarski · 2024-09-18T13:30:43Z

v1.1:

addressed @wprzytula comments
moved errors module to transport module (to retain the original path of error types: scylla::transport::errors
introduced get_error() public getter for ConnectionSetupRequestError to retrieve an error kind (the field itself is private)

Currently, most of the error types are contained in scylla-cql crate. This makes it much harder to make some errors pub(crate) (re-exporting is required). In addition, some error types, such as `QueryError` and `NewSessionError` should not belong to scylla-cql crate. They belong to public API and should be defined in `scylla` crate.

In addition, marked it as non_exhaustive.

We want to move `UserRequestError` to scylla crate. To achieve this, we cannot have any places in `scylla-cql` crate that would depend on this error's definition. Fortunately, there was only one place that needed it. To fix it, we simply narrow the return type of the method and introduce a From<> implementation for the transition. This way, we do not have to adjust the callers (that use `?` operator).

This error type is used only for driver's internal error management. It's not intended to be exposed to users.

The only reason it was firstly defined in `scylla-cql` is because `QueryError` and `NewSessionError` were defined there - they directly depend on this type. This is no longer true, since we moved both of these errors to scylla crate.

Again, it was defined in scylla-cql crate because ConnectionPoolError directly depends on it.

muzarski · 2024-09-18T13:44:36Z

Rebased on main.

scylla/src/errors.rs

muzarski · 2024-09-18T16:36:54Z

v1.2: addressed @Lorak-mmk comments

made BrokenConnectionErrorKind, CqlEventHandlingError and RequestError public
provided docstrings for the types mentioned above
adjusted error reason getters for BrokenConnectionError

Lorak-mmk · 2024-09-18T17:05:53Z

I see you introduced a new method:

    /// Retrieve a typed error reason.
    pub fn get_reason_typed(&self) -> &BrokenConnectionErrorKind {
        self.0.downcast_ref::<BrokenConnectionErrorKind>().unwrap()
    }

I believe it should not be present.

It lifts the burden of downcasting from user, which is not the goal.
Prevents us from changing the type later. If we just return Arc<dyn Error> we can change the underlying error type later without breaking semver. With this method present that is no longer possible.

muzarski · 2024-09-18T17:12:28Z

I see you introduced a new method:

    /// Retrieve a typed error reason.
    pub fn get_reason_typed(&self) -> &BrokenConnectionErrorKind {
        self.0.downcast_ref::<BrokenConnectionErrorKind>().unwrap()
    }

I believe it should not be present.

1. It lifts the burden of downcasting from user, which is not the goal.

2. Prevents us from changing the type later. If we just return `Arc<dyn Error>` we can change the underlying error type later without breaking semver. With this method present that is no longer possible.

Makes sense, I'll remove it.

Changed the name from `get_inner()` to `get_reason()`.

This error type is intended for driver's use only.

In addition, marked it as `non_exhaustive`.

AddrParseError does not include an information about the string that we tried to parse. This is why we include it in our error type.

In addition, marked it as non_exhaustive.

It should not belong to errors module.

…::errors

The only error type (and its dependencies) that was left in this module was DbError type. It directly corresponds to `response::error::Error` type. I think the `response::error` module is a great place to put it there. We re-export this type in scylla crate, so it can be found in the same place as other error types (scylla::errors, previously scylla::transport::errors). We can also now remove the scylla-cql::errors module altogether.

…n]Error This is now included in `BrokenConnectionErrorKind`. Corresponding variant in [Query/NewSession]Error is never constructed.

muzarski · 2024-09-18T17:15:09Z

I see you introduced a new method:

    /// Retrieve a typed error reason.
    pub fn get_reason_typed(&self) -> &BrokenConnectionErrorKind {
        self.0.downcast_ref::<BrokenConnectionErrorKind>().unwrap()
    }

I believe it should not be present.

1. It lifts the burden of downcasting from user, which is not the goal.

2. Prevents us from changing the type later. If we just return `Arc<dyn Error>` we can change the underlying error type later without breaking semver. With this method present that is no longer possible.

Makes sense, I'll remove it.

Done

Originally, all errors could be found under scylla::transport::errors. Let's retain the same path.

muzarski · 2024-09-18T17:20:50Z

v1.3: fixed a docstring comment of RequestError::UnableToAllocStreamId.

muzarski self-assigned this Sep 18, 2024

muzarski requested review from wprzytula and Lorak-mmk September 18, 2024 03:38

muzarski mentioned this pull request Sep 18, 2024

errors: refactor error types on a connection level #1067

Merged

7 tasks

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Sep 18, 2024

muzarski force-pushed the error-types-visibility branch from b298a89 to 09735bc Compare September 18, 2024 03:47

muzarski force-pushed the error-types-visibility branch from 3cd3611 to cf46893 Compare September 18, 2024 06:10

muzarski force-pushed the error-types-visibility branch from cf46893 to 08fb8e6 Compare September 18, 2024 06:13

wprzytula requested changes Sep 18, 2024

View reviewed changes

wprzytula added this to the 0.15.0 milestone Sep 18, 2024

muzarski force-pushed the error-types-visibility branch 2 times, most recently from aecf9a1 to 3ecb7f0 Compare September 18, 2024 08:24

muzarski requested a review from wprzytula September 18, 2024 13:25

muzarski added 10 commits September 18, 2024 15:44

errors: move QueryError to scylla crate

419adab

In addition, marked it as non_exhaustive.

errors: move NewSessionError to scylla crate

5fc5fc3

In addition, marked it as non_exhaustive.

errors: remove unused IoError -> NSError conversion

900cc23

errors: move UserRequestError to scylla crate

3e25c75

errors: make UserRequestError pub(crate)

e172138

This error type is used only for driver's internal error management. It's not intended to be exposed to users.

errors: move ConnectionPoolError to scylla crate

8824494

The only reason it was firstly defined in `scylla-cql` is because `QueryError` and `NewSessionError` were defined there - they directly depend on this type. This is no longer true, since we moved both of these errors to scylla crate.

errors: move ConnectionError to scylla crate

7b88721

Again, it was defined in scylla-cql crate because ConnectionPoolError directly depends on it.

errors: move ConnectionSetupRequestError to scylla crate

f8c55d6

muzarski force-pushed the error-types-visibility branch from 3ecb7f0 to c1c172b Compare September 18, 2024 13:44

Lorak-mmk requested changes Sep 18, 2024

View reviewed changes

scylla/src/errors.rs Outdated Show resolved Hide resolved

muzarski requested a review from Lorak-mmk September 18, 2024 16:38

muzarski added 14 commits September 18, 2024 19:14

errors: adjust getters for error reason of BCError

178f4e4

Changed the name from `get_inner()` to `get_reason()`.

errors: add docstring for BrokenConnectionError

44d5a61

errors: move CqlEventHandlingError to scylla crate

846d003

errors: move ResponseParseError to scylla crate

b59f6c0

errors: make ResponseParseError pub(crate)

7f0c4db

This error type is intended for driver's use only.

errors: move Translation error to scylla crate

4038602

In addition, marked it as `non_exhaustive`.

errors: add more context to TranslationError

79eba8b

AddrParseError does not include an information about the string that we tried to parse. This is why we include it in our error type.

errors: move BadQuery to scylla crate

d30e2b5

In addition, marked it as non_exhaustive.

errors: move BadKeyspaceName to scylla crate

eaa30fa

In addition, marked it as non_exhaustive.

request: move CqlRequestKind enum to request module

b7c6033

It should not belong to errors module.

response: move CqlResponseKind to response module

7853c0f

It should not belong to errors module.

errors: move module-level docstring from scylla_cql::errors to scylla…

332d0dd

…::errors

errors: remove TooManyOrphanedStreamIds variant from [Query/NewSessio…

e5f879e

…n]Error This is now included in `BrokenConnectionErrorKind`. Corresponding variant in [Query/NewSession]Error is never constructed.

muzarski force-pushed the error-types-visibility branch from 043e73f to 0720d4b Compare September 18, 2024 17:14

muzarski added 3 commits September 18, 2024 19:20

errors: add docstrings for public enum variants

4786c01

errors: adjust docstring Session reference in module docstring

bef83fc

errors: move errors module to transport module

601417c

Originally, all errors could be found under scylla::transport::errors. Let's retain the same path.

muzarski force-pushed the error-types-visibility branch from 0720d4b to 601417c Compare September 18, 2024 17:20

Lorak-mmk approved these changes Sep 18, 2024

View reviewed changes

wprzytula approved these changes Sep 19, 2024

View reviewed changes

wprzytula merged commit 44a3093 into scylladb:main Sep 19, 2024
11 checks passed

wprzytula mentioned this pull request Nov 14, 2024

Release 0.15 #1125

Merged

muzarski deleted the error-types-visibility branch December 19, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: move non-frame error types to scylla crate and adjust their visibility #1074

errors: move non-frame error types to scylla crate and adjust their visibility #1074

muzarski commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

wprzytula commented Sep 18, 2024 •

edited

Loading

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

Lorak-mmk commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

errors: move non-frame error types to scylla crate and adjust their visibility #1074

errors: move non-frame error types to scylla crate and adjust their visibility #1074

Conversation

muzarski commented Sep 18, 2024 • edited Loading

Motivation

Changes

Questions

Pre-review checklist

github-actions bot commented Sep 18, 2024 • edited Loading

wprzytula commented Sep 18, 2024 • edited Loading

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

Lorak-mmk commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024

muzarski commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

wprzytula commented Sep 18, 2024 •

edited

Loading